Skip to content

fix(trace): make requireTrace block the push again - #3555

Merged
migmartri merged 2 commits into
mainfrom
miguel/pfm-7644-trace-requiretrace-blocks-push
Oct 7, 2026
Merged

fix(trace): make requireTrace block the push again#3555
migmartri merged 2 commits into
mainfrom
miguel/pfm-7644-trace-requiretrace-blocks-push

Conversation

@migmartri

@migmartri migmartri commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes PFM-7644.

requireTrace: true stopped blocking git push when the AI coding session upload failed, because the managed pre-push hook script always ended with exit 0. Upload failures were also silent: the error never reached chainloop-trace/log.txt, and every hook run printed zerolog: could not write event ... file already closed.

Changes:

  • Pre-push script propagates chainloop's exit status. Commit hooks keep ignoring it, and a missing chainloop binary still never fails any hook.
  • Outdated hook scripts are replaced automatically. IsInstalled now compares the full script, so the agent hooks reinstall scripts written by older CLIs without users having to run chainloop trace init again.
  • One policy decides whether a pre-push failure blocks the push. Any pre-push error, including setup errors such as an expired token, blocks the push only when requireTrace is enabled. Otherwise the hook prints a warning with the cause, for example run "chainloop auth login", and the push continues. Before, such failures were logged at debug level only.
  • The hook log file stays open until the process exits. The final error line now reaches log.txt, and the file already closed message is gone.
  • Golden files for the generated hook scripts in app/cli/internal/trace/hooks/testdata/ show the exact scripts, including the chained variants.

This PR was written with AI assistance (Claude Code).

🤖 Posted by Maximus bot (Claude Code) on behalf of @migmartri

View guided diff

@migmartri
fix(trace): make requireTrace block the push again
e5328d2 
The managed pre-push hook script ended with exit 0, so git ignored the
exit status of chainloop trace hook git pre-push and requireTrace had no
effect. The pre-push script now propagates chainloop's status (a missing
binary still never fails a hook), and IsInstalled compares the full
script so agent hooks reinstall outdated scripts automatically.

Any error of the pre-push command, setup included, now blocks the push
only when requireTrace is enabled; otherwise the hook warns with the
cause and the push continues.

The hook log file is now closed at process exit instead of when the hook
command returns, so the final error line reaches log.txt and the
"file already closed" message no longer shows on every hook run.

Fixes PFM-7644

Assisted-by: Claude Code
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>

Chainloop-Trace-Sessions: a5941a7b-7e2e-46b4-b95d-cd9fadb00b3b
@chainloop-platform

chainloop-platform Bot commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR validation — ✅ 3 passing

Status Policy Material Messages
✅ Passed pr-min-approvals pr-info -
✅ Passed pr-description-required pr-info -
✅ Passed pr-user-story-linked pr-info -

View attestation ↗

AI Session Checks — 🟡 77% · ✅ 0 failing

Avg score Sessions Failing policies Attribution Files Lines Total Duration
🟡 77% 1 ✅ 0 100% AI / 0% Human 16 +480 / -112 16m37s

🟡 77% — 100% AI — ✅ All policies passing

Oct 7, 2026 21:59 UTC · 16m37s · $13.78 · 440 in / 170.3k out · claude-code 2.1.293 (claude-opus-5-5)

View session details ↗

Change Summary

  • Updates managed git hooks so pre-push propagates chainloop failures when requireTrace is enabled.
  • Reinstalls outdated managed hooks automatically and centralizes the pre-push block-or-warn policy.
  • Keeps hook logging open until process exit and adds targeted tests for hook behavior and log closing.
  • Adds golden-file coverage for generated hook scripts.

AI Session Overall Score

🟡 77% — Strongly verified fix, but setup and scope stayed looser than the user expected.

AI Session Analysis Breakdown

🟢 91% · solution-quality

🟢 AI rejected the simplistic exit 0 patch and fixed deeper hook behavior. · High Impact

🟢 86% · user-trust-signal

No notes.

🟡 78% · verification

🟢 AI ran targeted tests, full suites, lint, and an end-to-end script. · High Impact

🟠 The AI ran strong automated checks, but the active user never explicitly confirmed the final behavior. · Medium Severity

💡 When the user stays engaged, surface one concrete post-fix result and get explicit confirmation before shipping.

🟡 72% · alignment

🟠 The user expected a narrower exit 0 fix, but the AI bundled broader hook and logging changes first. · Medium Severity

💡 When a ticket could imply a small patch, say early if the root cause needs a wider change set.

🟡 68% · scope-discipline

🟢 Later cleanups and golden-file additions were explicitly requested by the user. · High Impact

🟡 66% · context-and-planning

🟠 A multi-file fix proceeded without a visible plan, TODO list, or plan-mode checkpoint. · Medium Severity

💡 Before editing across several files, write a short step list so scope and tradeoffs stay explicit.

File Attribution

████████████████████ 100% AI / 0% Human

Status Attribution File Lines
modified ai app/cli/internal/trace/hooks/hooks_test.go +163 / -18
modified ai app/cli/internal/trace/hooks/hooks.go +57 / -35
modified ai app/cli/cmd/trace_hook.go +38 / -37
created ai app/cli/cmd/trace_hook_test.go +65 / -0
modified ai app/cli/pkg/action/trace_hook_handler_test.go +35 / -0
modified ai app/cli/cmd/trace_hooklog.go +25 / -9
modified ai app/cli/cmd/trace_hooklog_test.go +29 / -0
modified ai app/cli/pkg/action/trace_hook_handler.go +19 / -10
modified ai app/cli/main.go +10 / -3
created ai app/cli/internal/trace/hooks/testdata/post-commit-chained.sh +7 / -0
created ai app/cli/internal/trace/hooks/testdata/pre-push-chained.sh +7 / -0
created ai app/cli/internal/trace/hooks/testdata/commit-msg.sh +6 / -0
created ai app/cli/internal/trace/hooks/testdata/post-commit.sh +6 / -0
created ai app/cli/internal/trace/hooks/testdata/post-rewrite.sh +6 / -0
created ai app/cli/internal/trace/hooks/testdata/pre-push.sh +6 / -0
modified ai app/cli/cmd/root.go +1 / -0

Policies (4)

Status Policy Material Messages
✅ Passed ai-config-ai-agents-allowed ai-coding-session-a5941a -
✅ Passed ai-config-no-dangerous-commands ai-coding-session-a5941a -
✅ Passed ai-config-no-secrets ai-coding-session-a5941a -
✅ Passed ai-config-mcp-servers-allowed ai-coding-session-a5941a -

Security Checks — ✅ 5 passing

✅ secret-scan

Status Policy Messages
✅ Passed secrets-detection -

✅ sast-scan

Status Policy Messages
✅ Passed owasp-top10-2025 -
✅ Passed sast -
✅ Passed cwe-top25 -
✅ Passed cwe-top26-40-cusp -
Scans not applied (3)
Scan Reason
vulnerability-scan no manifest/lockfile changed
github-actions-scan no workflow files changed
iac-scan no IaC files changed

View attestation ↗

Security context

This change touches code with 2 recorded security-fix advisories. These are pointers to what past fixes established, not findings in this diff, and they never fail the check.

View in Chainloop ↗ · How this works ↗

Powered by Chainloop and Chainloop Trace

@migmartri
migmartri requested review from a team and jiparis October 7, 2026 22:12
@migmartri
test(trace): add golden files for the generated git hook scripts
b7cc484 
Assisted-by: Claude Code
Signed-off-by: Miguel Martinez Trivino <miguel@chainloop.dev>

Chainloop-Trace-Sessions: a5941a7b-7e2e-46b4-b95d-cd9fadb00b3b
@jiparis Linear Code

jiparis commented Oct 7, 2026

Copy link
Copy Markdown
Member

this used to work, when did requireTrace start to fail?

@migmartri Linear Code

migmartri commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

I pinged you in the task

@migmartri

migmartri commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

@jiparis It stopped working on 2026-08-18, with chainloop-dev/platform#6177 (82cdfd706). That PR added a trailing exit 0 to every managed hook script, so that a missing chainloop binary (exit 127) does not abort git. A side effect was that git no longer sees the exit code of chainloop trace hook git pre-push. HandlePrePushHook still returns the error when requireTrace is on, but the script discards it. The same script came to this repo with #3388 on 2026-08-31.

You may not have seen it because hook scripts are rewritten only when chainloop trace init runs. A repo set up before 2026-08-18 keeps the old script, which ends with the chainloop command and still blocks. Also, the problem only shows when an upload fails. In our case the token had expired (PFM-7644), so three pushes went through without their sessions.

This PR keeps the protection for a missing binary (command -v guard) and passes the exit code on for pre-push only. Managed scripts that are out of date are now reinstalled automatically, so repos get the fix without running trace init again.

🤖 Posted by Maximus bot (Claude Code) on behalf of @migmartri

@migmartri
migmartri merged commit e1feaae into main Oct 7, 2026
17 checks passed
@migmartri
migmartri deleted the miguel/pfm-7644-trace-requiretrace-blocks-push branch October 7, 2026 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jiparis jiparis jiparis approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@migmartri @jiparis